feat(read-file): record read scope and report clipping separately (U4, #1375) - #1913

Pre-merge review flagged that the write path trusts an unvalidated symlink target: safeWriteText resolves an
existing link to its referent and safeWriteJson then merges and publishes onto that referent, so a repository that
plants its project settings file as a link elsewhere receives the write at the linked path.

safeWriteJson now accepts confineTo. The check runs on the resolved publish target, after resolvePublishTarget and
before the merge read and before anything is staged, so a rejected write takes no lock and leaves no residue. Both
sides are canonicalized the same way: the scope is resolved through symlinks (walking to the nearest existing
ancestor when it does not exist yet, and rethrowing anything other than ENOENT), and the target is canonicalized
with the same helper because a target that does not exist yet still carries the alias components of the path it was
given.

Callers that picked a path from a known scope pass that scope; a caller that does not declare one keeps the current
behaviour. The MCP settings wiring that uses this is on the async-save series (Zoo-Code-Org#1403).

Tests: an out-of-scope path is rejected on every platform with no lock and no staged file; a planted link escaping
the project is rejected and the linked file keeps its bytes; a link inside the project still publishes to the
referent; and a scope declared through a symlinked directory that does not exist yet is accepted.

The staging-identity guard compared the caller-staged file with the target through fs.lstat(targetPath).catch(() =>
null). Any error other than ENOENT - EACCES, ENOTDIR, ELOOP - was therefore indistinguishable from "there is no
target", so a staging path that is a hard link of the target would pass the identity check, reach the commit rename,
and let the failure handler unlink the only copy of the content.

Only ENOENT now yields "no target"; anything else fails closed with a StagingPathError before anything is opened,
staged or renamed. Test: the staging path resolves to a hard link of the target (same ino/dev) and the target lstat
rejects with EACCES; the write is rejected with the comparison error and neither openSync nor rename is called.

The staging-identity guard read fs.Stats.ino/dev as JS numbers. On NTFS and ReFS those identifiers can exceed
Number.MAX_SAFE_INTEGER, and the rounding makes two different files look identical - a valid caller-staged file is
then rejected with StagingPathError (and a real alias could be missed). Both lstats now request { bigint: true } and
the comparison checks for bigint values before comparing them.

The spec stand-in carries bigint ino/dev, matching what lstat({ bigint: true }) returns at runtime (fs.BigIntStats is
a type-only export, so the double is documented at its single assertion).

The identity stand-in returns bigint identifiers regardless of the options, so the same-file tests could still pass if
either lstat dropped { bigint: true } - which is exactly the case that matters on NTFS/ReFS. Both tests now assert
that every identity lstat requested bigint stats.

The calls are filtered by their options rather than by path spelling: path.resolve prefixes a drive letter to
/tmp/... on Windows, so a path filter would only see one of the two reads on that platform.

…served

On win32 the DACL of an existing target is saved before the commit rename so it can be reapplied afterwards. Two
paths silently skipped that step and still published:
- icacls /save failed (saved === false): the dump is cleaned up and the rename proceeds, so the new file inherits
  different access rights;
- fs.access(targetPath) failed with something other than ENOENT (EACCES, ...): the catch treated "cannot check" as
  "target absent" and skipped DACL handling entirely.

Both now report through a new onWarning sink (default console.warn): the write still proceeds - a missing or failing
icacls must not leave the user unable to save, which is the documented fallback - but the caller is told the
replacement may inherit different access rights instead of discovering it later.

Tests: icacls save failure still commits the write and yields exactly one access-rights warning; an EACCES on the
target yields the could-not-check warning and no icacls call. Verified as real regression tests - neutralizing the two
warn calls makes both fail. 272 passed / 4 skipped locally; tsc and eslint clean.

onWarning is documented as the sink for non-fatal safety notices, but delivery was not isolated from the write: an
onWarning callback that threw propagated to the outer failure handler before fs.rename, turning a non-fatal notice into a
failed save. The warn binding now catches callback failures and logs them. The restore-failure notice is routed through
the same binding so a caller supplying onWarning receives it. Same fix as the u6 unit, kept aligned across the series.

Local: safeWriteText 58/58 green; eslint clean, no suppression growth.

proper-lockfile creates ${lockKey}.lock beside the lock key, and the key is the
symlink referent. A repository that plants .roo/mcp.json -> ~/.ssh/config
therefore made a confined write create a lock directory OUTSIDE the declared
scope, and when that directory was not writable the caller got a
lock-acquisition error after up to five retries instead of
ConfinedPathEscapeError. The confinement check now runs on the lock key before
acquireFileLock and is repeated on the resolved publish target inside the lock
(a peer writer may have moved the referent in between); both checks share
_assertWithinScope so they canonicalize identically.

Ported across the file-safety unit series so every unit that declares a scope
carries the same guarantee (same fix as fws/u3-observation-completeness).

Series alignment for the two review findings fixed on fws/u6-apply-patch-wiring:

1. safeWriteText's warn wrapper could not catch a rejection from an async
   onWarning sink - TypeScript accepts a value-returning callback where a void one
   is expected - so the rejected promise was left unhandled, which under Node's
   default mode can end the process after a write that already succeeded. The
   wrapper now attaches a catch handler without awaiting (awaiting would let
   warning delivery delay a committed write, or stall it on a hung sink) and
   reports the rejection through the fallback sink.

2. safeWriteJson created the target's parent directory BEFORE the preflight
   confinement check, so a confined write to an out-of-scope path with a missing
   parent still created a directory outside confineTo. resolveLockKey and the check
   need no directory to exist, so the order is now lock key, confinement, mkdir;
   the in-lock check on the resolved publish target stays.

…ith doUnmock

- safeWriteText.integration.spec: the backup:true case bails out in the backup step (copyfile
  over a directory), so the commit rename never runs and a regression in commit-failure cleanup
  would pass. Added the backup:false case, which is the shape that reaches Step 4: the rename of
  the staging file over the target directory fails and the staging file must be cleaned up.
- safeWriteJson.test: the finally blocks removed a vi.doMock registration with vi.unmock, which
  vitest hoists to the top of the file, so the cleanup did not undo the registration where it
  was written. Both sites now use vi.doUnmock, the non-hoisted counterpart.

Verified the two paths are distinct by calling safeWriteText against a real directory target:
backup:true fails at copyfile, backup:false fails at rename of the staging file.
Local: 29 passed / 4 skipped across the two specs; eslint clean on both files.

Task.ts initializes the registry per Task, but nothing read it off a real Task: the registry
tests exercise standalone instances and ReadFileTool's spec injects its own double, so the
wiring itself was unverified - a Task that never built a registry, or shared one across
Tasks, would pass every existing test while silently dropping read observations.

The new test constructs a real Task and asserts observationRegistry is an ObservationRegistry,
that a second Task gets a different instance, and that an observation recorded on one Task is
invisible to the other (parent and subtask authority over a file must stay separate).

Local: Task.dispose.test.ts 15 passed; eslint clean; no new tsc diagnostics in this file.

Security Boundaries: safeWriteJson resolves the publish target through realpath
before staging beside it, so a repository that ships .roo/mcp.json as a symlink to
a file OUTSIDE the workspace has that outside file replaced as soon as the user
edits a project MCP setting, deletes a server, or toggles tool always-allow. The
three project-capable write sites passed no confinement option, and the base
implementation renamed the link itself, so the external write is introduced here.

confineForMcpWrite(source) returns the provider cwd (falling back to
getWorkspacePath) for project writes and undefined for global ones - global
settings live in the user's own settings directory, which is deliberately not
confined. All three sites now pass it, so the write fails closed with
ConfinedPathEscapeError instead of touching the referent. This is the same change
as 256091d on fws/u3-observation-completeness (Zoo-Code-Org#1912); the unit branches are not
cumulative, so every branch that carries the writer needs its own copy.

Regression Evidence: the per-Task registry now fails closed on teardown too -
disposeOnce() clears it, so a disposed Task still reachable through a
parent/subtask reference cannot hand a token captured before teardown to a
guarded write. Two McpHub tests pin the call sites (a project-scoped allowlist
write carries confineTo = workspace root, a global one does not) and a
Task.dispose test pins the clear. The two-real-Task registry test added in
87986e9 already covers the distinctness the checklist row asked for.

Local: vitest McpHub + Task.dispose 89 passed. Negative controls: helper mutated
to return undefined -> project confine test fails; clear() commented out ->
disposal test fails; both restore green. tsc --noEmit 50 errors before and after
the change (unchanged branch baseline). eslint 0 errors / 0 warnings on all four
files, no suppression entry added or increased.

…every MCP confine site

Lifecycle Resource Cleanup: disposeOnce() cleared the registry, but a read that was
already awaiting fs.readFile when disposal began resumed afterwards and recorded the
file again - the disposed Task was authoritative once more, and it kept doing the new
post-read stat work for a turn that will never be acted on. ObservationRegistry.close()
now drops the entries AND refuses later observations, Task.disposeOnce() calls close(),
and both ReadFileTool paths skip the post-read stat + observe work when task.abort is
set (checked before the awaited stat, not after it).

Regression Evidence: the confinement fix touched three call sites but only
toggleToolAlwaysAllow was pinned. updateServerTimeout and deleteServer now have a
project case (confineTo = workspace root) and a global case (confineTo undefined) each;
the connection double is shared at describe scope so all four sites use one typed
fixture. safeWriteJson is mocked in this file, so a lower-level writer test cannot see
a missing MCP argument - each call site has to be pinned here.

Tests: 2 closure tests (close drops entries and reports closed; observe is a no-op after
close), 2 aborted-task tests (native and legacy path record nothing), 4 call-site
confinement tests, and the disposal test extended to assert the registry stays empty
when a late observation arrives.

Local: 203 passed across readFileTool, McpHub, Task.dispose and observationRegistry.
Negative controls, each mutate -> test fails -> restore -> passes: ReadFileTool guard
neutered, ObservationRegistry closure guard neutered (fails both the registry spec and
the Task disposal test), and close() downgraded to clear() (fails the disposal test).
tsc --noEmit 50 errors before and after (unchanged branch baseline, 0 in touched files).
eslint 0 errors / 0 warnings across all 8 touched files; no suppression entry changed.

The abort guard sat before the post-read fs.stat, so a cancellation that
lands while that stat is in flight still recorded an observation - for a
run that will never act on the read, and into a registry that disposal has
already retired. Both paths now re-check task.abort after the await and
before observe(); the read itself still answers the model.

Two tests flip task.abort inside the pending post-read stat, one per path,
and assert observe() was never called and the registry stayed empty.

Port of the Zoo-Code-Org#1917 fix into this unit: the unit branches are not cumulative, so this
branch carries its own copy of safeWriteText's lock-key helper and the same defect.

canonicalDirKey() canonicalized only the immediate parent and fell back to that literal
spelling on ENOENT. A writer whose parent directory already existed canonicalized through
a symlinked ancestor (or a Windows short name) while a writer racing to create the same
directory got the literal path, so the two took different locks for one file and a
read-modify-write lost one side. It now walks up to the nearest ancestor that exists,
canonicalizes that, and re-joins the missing components; a realpath failure that is not
ENOENT is propagated instead of being papered over with a key that may be wrong.

Identifiers and comments are kept identical to the other units so the merge resolves
trivially.

… the write

A defect reported on a sibling PR in the base repo: the backup destination is named before
the copy runs, while the rollback cleanup keys off a flag that only becomes true once the
copy succeeded - so a copyFile that fails after creating the destination leaves a
half-written .bak beside the target forever.

This branch does not have that shape. The whole backup creation (seed open with "wx",
copyFile, chmod, fsync) is wrapped in a catch that unlinks the destination and clears
backupPath before rethrowing, so the cleanup keys off the attempt rather than off the
success. What was missing is coverage for the exact case the report describes: copyFile
failing with the destination already created. Only the fsync-failure variant was tested.

No production change. Negative control: deleting the cleanup unlink inside that catch fails
exactly two tests - this one and the existing "a failed backup flush is reported and leaves
no partial backup behind" - and restoring it leaves the file byte-identical.

No source change. CodeRabbit's change_assessment_commit for this PR has been frozen at
5596f3a since 15:30 across three heads (949c263, f1fb1b2, 7dec01b): three accepted
"@coderabbitai full review" requests (18:42:03, 20:02:18, 20:19:42) each re-rendered the
summarize against the OLD assessment and produced no review object at head, while the same
pool produced a real at-head verdict for another PR minutes later. The auto-review that a
push schedules recomputes the assessment for the new head, which the manual request does not.

Pre-merge table row "Regression Evidence" (ReadFileTool.ts:220-250, 828-890):
the native and legacy successful-read tests now count the fs.stat calls that
match - correct path AND { bigint: true } - instead of relying on membership,
which the read path's plain directory-check stat satisfies on its own. The
counted bigint stats must bracket the read (one before, one after), and the
plain directory-check stat must stay the only stat without options. In both
abort-before-post-stat tests the pre-read observation stat is still counted
(1, not 0) and placed before the read, while the post-read bigint stat must
never appear.

Load-bearing proof, measured per mutant with the full spec:
- bigint:true -> false at the native pre-read stat: reds exactly the native
  success + native abort tests (2).
- bigint:true -> false at the native post-read stat: reds exactly the native
  success test (1).
- legacy pre-read / post-read: mirror images (2 and 1).
- abort guard removed at each post-read site: reds exactly its abort test
  (1 each). All six mutants restored byte-for-byte (sha256 verified).

Inline thread (McpHub.spec.ts): the global allowlist test picked the first
safeWriteJson call whose path contains "mcp", which can be the
default-creation write getMcpSettingsFilePath makes when the settings file is
missing - it never passes confineTo, so a confinement root added to the real
allowlist write would go unnoticed. The test now names the exact call: every
write to the settings file (endsWith mcp_settings.json) is checked, matching
the timeout and delete tests. Controls: adding confineTo to the production
allowlist write reds exactly this test; with a decoy mcp-named write first,
the old find went red on the decoy while the fixed matcher stayed green
(77/77), proving the selection names the right call.

Gates: readFileTool.spec 99 passed; McpHub.spec 77 passed; eslint
--max-warnings=0 clean on both files; eslint-suppressions.json untouched.

The repo-wide prettier check added on main (commit 667ff58, run on the
merge commit) flags exactly three lines inside this PR's own diff: one
execute call in readFileTool.spec.ts and two safeWriteJson filters in
McpHub.spec.ts, all introduced by this branch's earlier commits (measured
with git blame at the previous head). Each line exceeds printWidth 120 and
prettier wraps it; nothing else changes.

Pure-formatting proof, per the ratified criteria: byte equality after
removing whitespace AND commas holds for both files (McpHub 86088 == 86088
whitespace-only; readFileTool 72964 == 72965 with the single added trailing
comma from the call wrap). No file outside this PR's diff is touched.
prettier --check now exits 0 on both files in the CI view (LF content, root
config); both specs pass (99 and 77) and eslint --max-warnings=0 is clean.

…ix unit files

The compile job starts with pnpm format:check (prettier --check . added to
main by commit 667ff58), and it evaluates the merge commit. CI listed
exactly these six files as dirty; all six are inside this pull request's
own diff and every offending line traces to commits of this branch
(verified with git blame at the previous head).

Content is prettier 3.8.4 output only. Proof: for each file, the blob
before and after are byte-equal after removing all whitespace characters
and all commas (the formatter's reflow adds and removes trailing commas);
the only other delta is one redundant pair of grouping parentheses around
an arrow-function conditional body in safeWriteText.spec.ts, which
prettier itself removes. No token other than whitespace, commas, and that
paren pair changed.

No behavior change; no production semantics touched.

The Windows platform-unit-test job failed test:misc on the merge tree:
integrations/misc/__tests__/indentation-reader-unicode.spec.ts, added to
main by pull 1960 (commit a101c61), asserts the reader result with
toEqual over the exact five-key object. This unit adds hasClippedLines to
readWithSlice results and to both readers' error returns, so three of
those assertions received a sixth key on the merge commit.

The offending expectations came from main, not from this branch; the fix
lands in this pull request's own diff because the gate runs on this
pull's merge commit. The preceding merge of upstream main is what lets
this edit land as a modification: without it the file exists only on main
and the same edit would surface as an add/add conflict and make the pull
dirty.

Assertions updated to the shipped contract: hasClippedLines true for the
two clipped-line slice results, false for the empty-input slice result
and for both reader error returns. The readWithIndentation success
results do not carry the field and their assertions are unchanged.

Negative control: replacing the produced value with undefined at the
success site reddens the two true-cases, and at the error sites reddens
the error test; restoring turns the file green (28 passed).

Both main and this branch added the same "import type { Task } from
../../task/Task" line to readFileTool.spec.ts at different positions, so
the textual merge kept two copies and the file failed to collect:
TS2300 Duplicate identifier 'Task' (vitest reported "Tests no tests").
The Windows job never reached this project because test:misc failed
first, so CI had not surfaced it yet.

Kept main's copy (the line main owns) and removed this branch's copy.
Spec passes after the fix: 106 tests green.

…e spec

Both main and this branch edited readFileTool.spec.ts, and the merged
file contains 94 explicit-any occurrences while the merged
eslint-suppressions.json still declared 96 (the byte-identical file on
the pull's merge ref has the same staleness; CI had not reached the Lint
step because the compile job died earlier). The suppression service
exits 2 on suppressions that no longer occur, so the merged tree's lint
step would have failed.

Measured with eslint itself (prune run reported 96 -> 94 for this file
only) and applied as a one-line byte-exact edit; the file keeps its
TAB indentation and trailing newline. The count decreased, never
increased. Full src lint now exits 0 with --max-warnings=0.

Both confinement checks in safeWriteJson tested options?.confineTo for truthiness,
so a caller passing "" skipped both gates and the write followed any symlink with
no scope check. The value occurs in practice: McpHub.confineForMcpWrite falls back
to getWorkspacePath(), which is "" while no workspace folder is open, so a
project-scoped MCP write ran unconfined even though the caller declared a scope.

Confinement is now decided once, up front. _declaredScopeRoot returns undefined
only when confineTo is absent, rejects a declared root that is empty or
whitespace-only with a ConfinedPathEscapeError naming the declared root, and both
confinement checks key off the single derived presence value instead of separate
truthiness tests. A declared-but-empty root fails closed before the lock key is
resolved, before the lock is taken, before any directory is created, and before
anything is staged.

Tests pin the empty-string case: the rejection names the declared empty root, the
write does not proceed, the target parent directory is not created, and a
whitespace-only root fails the same way.

confineTo was documented as refusing a write "that a planted symlink would land
somewhere else", which reads as containment for the whole write. It is not. Both
confinement checks decide from the final component as it resolves at their own
instant, and publication is path based - the parent-directory creation and the
commit rename both take a path - so an ancestor directory replaced by a link in
the window between the last check and the rename redirects the publish. Measured
outcomes of that window, on both platform shapes:

- the swapped-in directory already has a file at the target name: realpath
  resolves the target onto it, the staging identity guard rejects the commit with
  StagingPathError, and nothing is written on either side;
- it does not: realpath and lstat both report ENOENT, resolvePublishTarget falls
  back to the alias spelling, and the rename follows the swapped ancestor,
  publishing OUTSIDE the declared scope.

Handle-relative no-follow publication (openat/renameat over a directory fd) would
close the window; this runtime exposes neither call, so the claim is narrowed here
instead of being made true. No new option and no second confinement mechanism:
the checks, their order and their outcomes are unchanged.

The credential-carrying call sites keep the control and state their own limit in
the comment on the helper they call: the case a committed tree can actually plant
is a link at the settings file itself, which confineTo does refuse, so dropping
the option would give up a refusal that works today in order to close a window no
unit can close in this runtime.

Two tests pin the limit as an executable contract, one per outcome, with the
ancestor swapped from the merge callback so it moves inside the window rather than
hopefully so. Against a production that honoured the old claim - a confinement
re-check at the publish - exactly those two tests are red (2 failed / 29 passed /
4 skipped); against the shipped production they are green, because this change
narrows a claim and not a behaviour. Per-call-site mutants of the control each
redden only the test that names that call site (pre-lock check 2, declared-but-
empty root 3, each McpHub confineTo 1).

Local: safeWriteJson, safeWriteJson.lockKey, safeWriteText, safeWriteText.integration
and McpHub together 177 passed / 4 skipped; eslint --max-warnings=0 clean on all
three files with no suppression drift; tsc --noEmit 137 errors, none in a touched
file (junction-donor baseline); prettier --check clean on the three files after the
CRLF to LF rewrite.

See tracking item 6104592203.

…rved

The Security Boundaries pre-merge row reports that safeWriteText commits a
replacement when the Windows DACL capture fails - it only warns, although the
replacement may inherit different access rights - and that the same warn-only
shape covers a DACL restore that fails after the commit rename. A project
.roo/mcp.json with restrictive ACLs can hold MCP env or header secrets, so a
publish that silently widens who can read the file is not a save the caller can
trust.

Fail closed for an existing target whose DACL cannot be read or preserved, on the
shape the chain's earliest unit already ships:

- DaclInspectionError (phase "save" | "inspect") is thrown in step 2, before the
  commit rename, when icacls /save fails or the target exists but its access
  rights cannot be checked. Nothing is published, so the target keeps its content
  and its rights, and the failure handler still releases the staged file and this
  write's own staging directory.
- DaclRestoreError is thrown in step 5 when the saved dump cannot be put back, so
  no caller can observe success for a publish whose DACL was not preserved.
- The failure handler keeps the backup copy for a DaclRestoreError instead of
  removing it: that copy is the only artifact still carrying the access rights of
  the file the publish replaced, so deleting it would destroy the recovery path
  for exactly the fact the error reports. Its path is named through onWarning,
  which now carries only that leftover notice.

Tests pin each point at the primitive: the refusal with its phase and target path,
no rename and no warning for a write that did not happen, cleanup counted once,
the restore failure as an error with the dump still unlinked, the backup copy
retained and named, and the caller-staged shape safeWriteJson hands in. Negative
controls: four mutants, one per changed production call site, each reddening only
the tests that name that behaviour, plus a run of the new tests against the
pre-fix production file (10 red, 55 green).

Zoo-Code-Org#2038 turned @typescript-eslint/no-unused-vars back on in src and removed
the tree's dead bindings. Replaying this unit onto that main exposed the
bindings this unit's own code left unused once main's deletions landed:
drop the ReadFileTool/SafeWriteTextOptions/lockfile imports and the
never-read committed flag, prefix the four write-path destructurings whose
value no assertion reads, and restore the safeWriteJson import main pruned
(main had no reader; this unit's McpHub tests do).

No behavior change: 357 tests green across the unit's specs, eslint clean,
tsc adds no new errors beyond the pre-existing local donor-package drift.